POC of the auto unsubscribe functionality #1
Conversation
📝 WalkthroughWalkthroughThe changes extend the unsubscribe functionality by introducing an email parameter to the backend API and creating a new client-side action that invokes an external unsubscriber service before marking newsletters as unsubscribed, while adding configuration for the external service URL. Changes
Sequence DiagramsequenceDiagram
actor User
participant BulkUI as Bulk Unsubscribe UI
participant Hook as useUnsubscribe Hook
participant Action as unsubscribeAction
participant ExtAPI as External Unsubscriber API
participant NewsAPI as Newsletter Status API
User->>BulkUI: Trigger unsubscribe
BulkUI->>Hook: Call unsubscribe()
alt When lastUnsubscribeLink exists
Hook->>Action: Call unsubscribeAction({ url })
Action->>ExtAPI: POST with url and user email
ExtAPI-->>Action: Response status
Action-->>Hook: { success: true/false }
end
Hook->>NewsAPI: Call setNewsletterStatusAction()
NewsAPI-->>Hook: Status updated
Hook-->>User: Unsubscribe complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @apps/unsubscriber/src/main.ts:
- Around line 206-212: The email parameter on autoUnsubscribe is declared but
never used; either remove it from autoUnsubscribe's signature and all callers,
or propagate it into the helper calls that need user context: pass email into
analyzePageWithAI, performUnsubscribeActions, and performFallbackUnsubscribe
(and update those functions' signatures and internal logic to accept/use email
for form-filling or AI context). Ensure every call site and any types are
updated consistently to avoid breaking changes.
In @apps/web/app/(app)/bulk-unsubscribe/hooks.ts:
- Around line 60-62: The code is calling unsubscribeAction in the branch that
handles status === UNSUBSCRIBED (the re-subscribe/toggle-off flow), which is
wrong; update the logic so that when status === UNSUBSCRIBED you only set the
item status back to null (or perform the re-subscribe toggle) and do not call
unsubscribeAction, and move the await unsubscribeAction({ url:
item.lastUnsubscribeLink }) call into the else branch that handles the actual
unsubscription path; reference the status/UNSUBSCRIBED check, the
item.lastUnsubscribeLink usage, and the unsubscribeAction invocation in hooks.ts
to make this change.
- Around line 60-62: The code calls unsubscribeAction({ url:
item.lastUnsubscribeLink }) with no error handling; wrap that call in a
try/catch around unsubscribeAction (referencing unsubscribeAction and
item.lastUnsubscribeLink), on success proceed as before, on failure log the
error, surface a user-facing failure (e.g., return/throw an error or set a
failure status) and do not mark the item as unsubscribed; ensure the caller/UI
receives the error result so it can show a notification or retry option.
In @apps/web/utils/actions/unsubscriber.ts:
- Line 44: The current fetch call embeds session.user.email in the query string
(`${env.UNSUBSCRIBER_API_URL}/unsubscribe?url=${url}&email=${session.user.email}`),
exposing PII; change the request to POST to
`${env.UNSUBSCRIBER_API_URL}/unsubscribe` and move both url and
session.user.email into the JSON request body (e.g., { url, email:
session.user.email }), set the Content-Type: application/json header, and use
method: 'POST' so email is not leaked in logs or browser history; update the
corresponding call site in unsubscriber.ts (the fetch that builds that template
string) accordingly.
- Around line 43-45: The fetch currently issues a GET with raw query params
which mismatches the server's POST /unsubscribe and doesn't URL-encode inputs;
change the fetch in unsubscriber.ts to send a POST to
`${env.UNSUBSCRIBER_API_URL}/unsubscribe` with a JSON body containing url and
session.user.email (or the expected body shape), use encodeURIComponent if you
must place values in a URL, and set the appropriate header ('Content-Type':
'application/json'); ensure you JSON.stringify({ url, email: session.user.email
}) and handle the response as before.
- Around line 43-45: env.UNSUBSCRIBER_API_URL is optional but used directly in
the fetch call; add a guard that checks env.UNSUBSCRIBER_API_URL before building
the request (e.g., const base = env.UNSUBSCRIBER_API_URL) and if missing throw
or return a clear error message so the caller gets a helpful failure instead of
an unclear runtime error; then use that base when calling fetch and ensure you
encode the query params (url and session.user.email) when constructing the
request URL.
🧹 Nitpick comments (2)
apps/web/utils/actions/unsubscriber.ts (1)
43-49: Recommended: Add timeout and improve error handling.The fetch call lacks a timeout and doesn't handle network errors (fetch only rejects on network failures, not HTTP errors). Consider adding:
- A timeout using
AbortControlleror a timeout parameter- Try-catch to handle network failures
- More specific error messages based on response status
🔧 Suggested improvement
+ try { + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 30000); + const response = await fetch( `${env.UNSUBSCRIBER_API_URL}/unsubscribe`, + { + signal: controller.signal, + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ url, email: session.user.email }), + } ); + clearTimeout(timeoutId); if (!response.ok) { - return { error: "Failed to unsubscribe" }; + const message = await response.text().catch(() => 'Unknown error'); + return { error: `Failed to unsubscribe: ${message}` }; } return { success: true }; + } catch (error) { + if (error.name === 'AbortError') { + return { error: 'Unsubscribe request timed out' }; + } + return { error: 'Network error during unsubscribe' }; + }apps/unsubscriber/src/server.ts (1)
36-39: Optional: Update error message for clarity.The error message "Invalid URL provided" is now inaccurate since the schema validates both
urland✨ Suggested improvement
if (error instanceof z.ZodError) { - return reply.status(400).send({ error: "Invalid URL provided" }); + return reply.status(400).send({ error: "Invalid request parameters", details: error.errors }); }
📜 Review details
Configuration used: Environment YAML
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/unsubscriber/src/main.tsapps/unsubscriber/src/server.tsapps/web/app/(app)/bulk-unsubscribe/hooks.tsapps/web/env.tsapps/web/utils/actions/unsubscriber.ts
🔇 Additional comments (3)
apps/web/app/(app)/bulk-unsubscribe/hooks.ts (1)
82-82: LGTM!Correctly added
item.lastUnsubscribeLinkto the dependency array since it's now referenced in the callback.apps/web/env.ts (1)
81-82: LGTM!Adding
UNSUBSCRIBER_API_URLas an optional environment variable is appropriate for a POC, allowing the system to function without the external service configured.apps/unsubscriber/src/server.ts (1)
19-19: LGTM with a note.The server-side changes correctly accept and validate the email parameter. The POST endpoint properly extracts both
urlandHowever, note that the
autoUnsubscribefunction currently doesn't use the email parameter (see comment on apps/unsubscriber/src/main.ts).Also applies to: 28-29
| export async function autoUnsubscribe({ | ||
| url, | ||
| email, | ||
| }: { | ||
| url: string; | ||
| email: string; | ||
| }): Promise<boolean> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's see what references to email exist in the file
rg -n "email" apps/unsubscriber/src/main.tsRepository: admin-coderabbit/inbox-zero
Length of output: 220
🏁 Script executed:
#!/bin/bash
# Read the function implementation to see what it actually does
sed -n '206,276p' apps/unsubscriber/src/main.tsRepository: admin-coderabbit/inbox-zero
Length of output: 2021
🏁 Script executed:
#!/bin/bash
# Check the signatures of helper functions to see if they should receive email
rg -A 3 "function (analyzePageWithAI|performUnsubscribeActions|performFallbackUnsubscribe)" apps/unsubscriber/src/main.ts | head -30Repository: admin-coderabbit/inbox-zero
Length of output: 556
Critical: Unused parameter in function signature.
The email parameter is accepted in the function signature (line 208) but never used anywhere in the function body (lines 213-276). It's not passed to analyzePageWithAI, performUnsubscribeActions, performFallbackUnsubscribe, or any other helper function, and is never referenced in any form.
Either implement the email parameter by passing it to the appropriate helper functions (if needed for form filling or AI context), or remove it to avoid confusion and breaking changes.
🤖 Prompt for AI Agents
In @apps/unsubscriber/src/main.ts around lines 206 - 212, The email parameter on
autoUnsubscribe is declared but never used; either remove it from
autoUnsubscribe's signature and all callers, or propagate it into the helper
calls that need user context: pass email into analyzePageWithAI,
performUnsubscribeActions, and performFallbackUnsubscribe (and update those
functions' signatures and internal logic to accept/use email for form-filling or
AI context). Ensure every call site and any types are updated consistently to
avoid breaking changes.
| if (item.lastUnsubscribeLink) { | ||
| await unsubscribeAction({ url: item.lastUnsubscribeLink }); | ||
| } |
There was a problem hiding this comment.
Critical logic error: unsubscribeAction called in the wrong flow.
Lines 59-68 handle the case where the newsletter is already unsubscribed (status === UNSUBSCRIBED). This is the "re-subscribe" or "toggle off" path that sets the status back to null. Calling unsubscribeAction here attempts to unsubscribe a user who is already unsubscribed, which is incorrect.
The unsubscribeAction should instead be called in the else block (line 70) where actual unsubscription happens.
🐛 Proposed fix
if (item.status === NewsletterStatus.UNSUBSCRIBED) {
- if (item.lastUnsubscribeLink) {
- await unsubscribeAction({ url: item.lastUnsubscribeLink });
- }
-
await setNewsletterStatusAction({
newsletterEmail: item.name,
status: null,
});
await mutate();
} else {
+ if (item.lastUnsubscribeLink) {
+ await unsubscribeAction({ url: item.lastUnsubscribeLink });
+ }
await unsubscribeAndArchive(item.name, mutate, refetchPremium);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (item.lastUnsubscribeLink) { | |
| await unsubscribeAction({ url: item.lastUnsubscribeLink }); | |
| } | |
| if (item.status === NewsletterStatus.UNSUBSCRIBED) { | |
| await setNewsletterStatusAction({ | |
| newsletterEmail: item.name, | |
| status: null, | |
| }); | |
| await mutate(); | |
| } else { | |
| if (item.lastUnsubscribeLink) { | |
| await unsubscribeAction({ url: item.lastUnsubscribeLink }); | |
| } | |
| await unsubscribeAndArchive(item.name, mutate, refetchPremium); | |
| } |
🤖 Prompt for AI Agents
In @apps/web/app/(app)/bulk-unsubscribe/hooks.ts around lines 60 - 62, The code
is calling unsubscribeAction in the branch that handles status === UNSUBSCRIBED
(the re-subscribe/toggle-off flow), which is wrong; update the logic so that
when status === UNSUBSCRIBED you only set the item status back to null (or
perform the re-subscribe toggle) and do not call unsubscribeAction, and move the
await unsubscribeAction({ url: item.lastUnsubscribeLink }) call into the else
branch that handles the actual unsubscription path; reference the
status/UNSUBSCRIBED check, the item.lastUnsubscribeLink usage, and the
unsubscribeAction invocation in hooks.ts to make this change.
Missing error handling for external API call.
The unsubscribeAction call has no specific error handling. If the external unsubscriber service fails, the user receives no feedback, and the status update proceeds regardless. This could lead to inconsistent state where the UI shows "unsubscribed" but the actual unsubscribe action failed.
Consider adding explicit error handling and user notification.
🛡️ Suggested improvement
if (item.lastUnsubscribeLink) {
- await unsubscribeAction({ url: item.lastUnsubscribeLink });
+ const result = await unsubscribeAction({ url: item.lastUnsubscribeLink });
+ if (result.error) {
+ toast.error("Failed to unsubscribe. Please try again.");
+ setUnsubscribeLoading(false);
+ return;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/web/app/(app)/bulk-unsubscribe/hooks.ts around lines 60 - 62, The code
calls unsubscribeAction({ url: item.lastUnsubscribeLink }) with no error
handling; wrap that call in a try/catch around unsubscribeAction (referencing
unsubscribeAction and item.lastUnsubscribeLink), on success proceed as before,
on failure log the error, surface a user-facing failure (e.g., return/throw an
error or set a failure status) and do not mark the item as unsubscribed; ensure
the caller/UI receives the error result so it can show a notification or retry
option.
| const response = await fetch( | ||
| `${env.UNSUBSCRIBER_API_URL}/unsubscribe?url=${url}&email=${session.user.email}`, | ||
| ); |
There was a problem hiding this comment.
Critical: HTTP method mismatch and missing URL encoding.
The client makes a GET request with query parameters, but the server endpoint at /unsubscribe is defined as POST (see apps/unsubscriber/src/server.ts line 26). This will result in a 404 or method not allowed error.
Additionally, the url and email parameters are not URL-encoded, creating potential injection vulnerabilities and breaking when URLs contain special characters.
🔧 Proposed fix
- const response = await fetch(
- `${env.UNSUBSCRIBER_API_URL}/unsubscribe?url=${url}&email=${session.user.email}`,
- );
+ const response = await fetch(
+ `${env.UNSUBSCRIBER_API_URL}/unsubscribe`,
+ {
+ method: 'POST',
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify({ url, email: session.user.email }),
+ }
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const response = await fetch( | |
| `${env.UNSUBSCRIBER_API_URL}/unsubscribe?url=${url}&email=${session.user.email}`, | |
| ); | |
| const response = await fetch( | |
| `${env.UNSUBSCRIBER_API_URL}/unsubscribe`, | |
| { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ url, email: session.user.email }), | |
| } | |
| ); |
🤖 Prompt for AI Agents
In @apps/web/utils/actions/unsubscriber.ts around lines 43 - 45, The fetch
currently issues a GET with raw query params which mismatches the server's POST
/unsubscribe and doesn't URL-encode inputs; change the fetch in unsubscriber.ts
to send a POST to `${env.UNSUBSCRIBER_API_URL}/unsubscribe` with a JSON body
containing url and session.user.email (or the expected body shape), use
encodeURIComponent if you must place values in a URL, and set the appropriate
header ('Content-Type': 'application/json'); ensure you JSON.stringify({ url,
email: session.user.email }) and handle the response as before.
Major: Missing validation for optional environment variable.
env.UNSUBSCRIBER_API_URL is defined as optional in the environment configuration, but there's no check before using it. If it's not configured, the fetch will fail with a confusing error.
🛡️ Proposed fix
const { url } = options;
+ if (!env.UNSUBSCRIBER_API_URL) {
+ return { error: "Unsubscriber service not configured" };
+ }
+
const response = await fetch(
`${env.UNSUBSCRIBER_API_URL}/unsubscribe`,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/web/utils/actions/unsubscriber.ts around lines 43 - 45,
env.UNSUBSCRIBER_API_URL is optional but used directly in the fetch call; add a
guard that checks env.UNSUBSCRIBER_API_URL before building the request (e.g.,
const base = env.UNSUBSCRIBER_API_URL) and if missing throw or return a clear
error message so the caller gets a helpful failure instead of an unclear runtime
error; then use that base when calling fetch and ensure you encode the query
params (url and session.user.email) when constructing the request URL.
| const { url } = options; | ||
|
|
||
| const response = await fetch( | ||
| `${env.UNSUBSCRIBER_API_URL}/unsubscribe?url=${url}&email=${session.user.email}`, |
There was a problem hiding this comment.
Major: Sensitive data exposure in URL.
The user's email address is included in the URL query string. GET requests are logged in server logs, proxy logs, and browser history, exposing PII unnecessarily. Use a POST request with the email in the request body instead.
🤖 Prompt for AI Agents
In @apps/web/utils/actions/unsubscriber.ts at line 44, The current fetch call
embeds session.user.email in the query string
(`${env.UNSUBSCRIBER_API_URL}/unsubscribe?url=${url}&email=${session.user.email}`),
exposing PII; change the request to POST to
`${env.UNSUBSCRIBER_API_URL}/unsubscribe` and move both url and
session.user.email into the JSON request body (e.g., { url, email:
session.user.email }), set the Content-Type: application/json header, and use
method: 'POST' so email is not leaked in logs or browser history; update the
corresponding call site in unsubscriber.ts (the fetch that builds that template
string) accordingly.
Hasn't been tested. Reference point to start the task yourself.
Summary by CodeRabbit
Release Notes
New Features
Updates
✏️ Tip: You can customize this high-level summary in your review settings.